Use _RO accessors for const targets #7611
Conversation
MichaelChirico
requested review from
HughParsonage,
ben-schwen and
tdhock
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7611 +/- ##
==========================================
- Coverage 99.02% 99.02% -0.01%
==========================================
Files 87 87
Lines 16903 16890 -13
==========================================
- Hits 16739 16726 -13
Misses 164 164 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No obvious timing issues in HEAD=ro-access Generated via commit 9b034c8 Download link for the artifact containing the test results: ↓ atime-results.zip
|
|
Another big class of cases where we could switch to There are roughly 240 such sites ( |
|
I see this has some (or total) overlap with #7394. I think it's good to break out the changes into minimally-digestible parts -- here we focus only on |
|
Should we add an accessor for #define INTEGER64_RO(x) ((const int64_t *) DATAPTR_RO(x)) |
Good point, though I think out of scope -- #7618 |
Co-authored-by: Benjamin Schwendinger <[email protected]>
| const R_xlen_t n = xlength(x); | ||
| if (n==0) | ||
| return ScalarInteger(0); // empty vector | ||
| int first = LOGICAL(x)[0]; |
There was a problem hiding this comment.
I know this PR is about smth else but just spotted this :/
should be bool. same applies for second and third
There was a problem hiding this comment.
I don't think that's right -- see below first==NA_INTEGER, that's not possible with bool.
Maybe I'm missing something.
There was a problem hiding this comment.
Good point! But then still should be NA_LOGICAL. Also only cosmetics since R defines like this
#define NA_LOGICAL R_NaInt
#define NA_INTEGER R_NaIntThere was a problem hiding this comment.
forgive me, I still don't quite follow... int is the correct type for LOGICAL(), if we implicitly cast it to bool, then if first is TRUE or NA, first == NA_INTEGER will either pass (by (bool)NA_INTEGER) or fail (by (int)(bool)first turning NA into 1).
There was a problem hiding this comment.
It could be Rboolean. Those are guaranteed to fit all of TRUE, FALSE, NA_LOGICAL.
There was a problem hiding this comment.
I thought the intention of Rboolean was to just provide bool:
There was a problem hiding this comment.
https://stat.ethz.ch/pipermail/r-devel/2025-January/083813.html
(next message is the next month so link isn't available there)
https://stat.ethz.ch/pipermail/r-devel/2025-February/083816.html
There was a problem hiding this comment.
Sorry, I forgot the outcome of that episode, you're right. int is appropriate, bool and Rboolean aren't supposed to handle NA_LOGICAL.
|
This Coccinelle spatch should at least help as a linter:
@@
type T;
const T *variable;
expression E;
@@
- variable = REAL(E)
+ variable = REAL_RO(E)
@@
type T;
const T *variable;
expression E;
@@
- variable = INTEGER(E)
+ variable = INTEGER_RO(E)
@@
type T;
const T *variable;
expression E;
@@
- variable = COMPLEX(E)
+ variable = COMPLEX_RO(E)
@@
type T;
const T *variable;
expression E;
@@
- variable = RAW(E)
+ variable = RAW_RO(E)
@@
type T;
const T *variable;
expression E;
@@
- variable = LOGICAL(E)
+ variable = LOGICAL_RO(E)
|
Co-authored-by: Benjamin Schwendinger <[email protected]>
Keep the casts because they are currently necessary for the integer64 case.
3 participants
Searched the sources:
grep -Er "const\b.*[*]\w+\s*=\s*[A-Z]+([^OR]|[^A]R)[(]" srcThis is some low-hanging fruit in terms of better use of read-only accessors. There will be other cases where we can make the change to a
constpointer as well, but those can't be identified with a simple regex.